[SPARK-27961][SQL] DataSourceV2Relation should not have refresh method by gengliangwang · Pull Request #24815 · apache/spark

gengliangwang · 2019-06-06T18:11:08Z

What changes were proposed in this pull request?

The newly added Refresh method in PR #24401 prevented the work of moving DataSourceV2Relation into catalyst. It calls case table: FileTable => table.fileIndex.refresh() while FileTable belongs to sql/core.

More importantly, Ryan Blue pointed out DataSourceV2Relation is immutable by design, it should not have refresh method.

How was this patch tested?

Unit test

gengliangwang · 2019-06-06T18:12:18Z

@rdblue @jzhuge @gatorsmile

rdblue · 2019-06-06T18:13:42Z

@gengliangwang, the catalog interface has a method to invalidate a source table. Do you want to implement REFRESH TABLE for v2 tables as a follow-up?

rdblue · 2019-06-06T18:14:54Z

+1 from me if tests pass. (I saw the comment about the Paruqet -> ORC fix on the previous PR.)

gengliangwang · 2019-06-06T18:16:22Z

@gengliangwang, the catalog interface has a method to invalidate a source table. Do you want to implement REFRESH TABLE for v2 tables as a follow-up?

Yes, I will create a follow-up. This one is to unblock the work of moving DataSourceV2Relation into catalyst.

dongjoon-hyun · 2019-06-06T18:34:00Z

@gengliangwang . Could you update the PR description a little bit? me is you or John?

dongjoon-hyun

Logically, this reverts [SPARK-27504][SQL] File source V2: support refreshing metadata cache.

I have two questions.

Are we going to make https://issues.apache.org/jira/browse/SPARK-27504 as Invalid?
Why not reverting the invalid SPARK-27504 cleanly?

cc @gatorsmile

dongjoon-hyun · 2019-06-06T18:43:54Z

sql/core/src/test/scala/org/apache/spark/sql/MetadataCacheSuite.scala

@@ -41,7 +41,7 @@ abstract class MetadataCacheSuite extends QueryTest with SharedSQLContext {
  test("SPARK-16336,SPARK-27504 Suggest doing table refresh " +
    "when encountering FileNotFoundException") {


The test case name Suggest doing table refresh is not valid anymore DSv2 after this PR. Maybe, recreating is better?

gengliangwang · 2019-06-06T18:44:23Z

@dongjoon-hyun I think we can partially revert it. Some of the works are still valuable:

Showing warning message in file source v2 if the underlying files have been updated
The test suite is reserved. Otherwise, we can see test failure in the Parquet V2 migration PR.

SparkQA · 2019-06-06T21:23:23Z

Test build #106248 has finished for PR 24815 at commit 8ab2ae5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-06-06T21:56:12Z

Test build #106252 has finished for PR 24815 at commit 626127d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/core/src/test/scala/org/apache/spark/sql/MetadataCacheSuite.scala

SparkQA · 2019-06-07T07:05:02Z

Test build #106267 has finished for PR 24815 at commit 98c6105.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

gengliangwang · 2019-06-07T07:08:45Z

retest this please.

SparkQA · 2019-06-07T10:07:38Z

Test build #106271 has finished for PR 24815 at commit 98c6105.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gengliangwang · 2019-06-08T05:52:11Z

@dongjoon-hyun Any other concerns on this PR?

dongjoon-hyun · 2019-06-08T06:36:43Z

Sorry for the delay, @gengliangwang . Yes, for now, this might be the less noisy path because SPARK-27504 was merged long time ago.

dongjoon-hyun · 2019-06-08T06:36:50Z

Retest this please.

SparkQA · 2019-06-08T07:05:01Z

Test build #106296 has finished for PR 24815 at commit 98c6105.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-06-08T07:13:43Z

Retest this please.

SparkQA · 2019-06-08T10:11:59Z

Test build #106300 has finished for PR 24815 at commit 98c6105.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun

+1, LGTM. Merged to master.
cc @gatorsmile since this PR has a history and the context.

jzhuge · 2019-06-09T23:48:11Z

Thanks @gengliangwang @rdblue @dongjoon-hyun for the quick resolution!

## What changes were proposed in this pull request? The newly added `Refresh` method in PR apache#24401 prevented the work of moving DataSourceV2Relation into catalyst. It calls `case table: FileTable => table.fileIndex.refresh()` while `FileTable` belongs to sql/core. More importantly, Ryan Blue pointed out DataSourceV2Relation is immutable by design, it should not have refresh method. ## How was this patch tested? Unit test Closes apache#24815 from gengliangwang/removeRefreshTable. Authored-by: Gengliang Wang <gengliang.wang@databricks.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

SPARK-27961 DataSourceV2Relation should not have refresh method

8ab2ae5

gengliangwang changed the title ~~[WIP][SPARK-27961][SQL] DataSourceV2Relation should not have refresh method~~ [SPARK-27961][SQL] DataSourceV2Relation should not have refresh method Jun 6, 2019

dongjoon-hyun requested changes Jun 6, 2019

View reviewed changes

dongjoon-hyun reviewed Jun 6, 2019

View reviewed changes

update test case name

626127d

jzhuge approved these changes Jun 6, 2019

View reviewed changes

dongjoon-hyun reviewed Jun 7, 2019

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/MetadataCacheSuite.scala Outdated Show resolved Hide resolved

revise test case name

98c6105

dongjoon-hyun approved these changes Jun 8, 2019

View reviewed changes

dongjoon-hyun closed this in db0f6b4 Jun 8, 2019

jzhuge mentioned this pull request Jun 9, 2019

[SPARK-27322][SQL] DataSourceV2 table relation #24741

Closed

dongjoon-hyun added the SQL label Feb 5, 2020

		@@ -41,7 +41,7 @@ abstract class MetadataCacheSuite extends QueryTest with SharedSQLContext {
		test("SPARK-16336,SPARK-27504 Suggest doing table refresh " +
		"when encountering FileNotFoundException") {

Conversation

gengliangwang commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gengliangwang commented Jun 6, 2019

Uh oh!

rdblue commented Jun 6, 2019

Uh oh!

rdblue commented Jun 6, 2019

Uh oh!

gengliangwang commented Jun 6, 2019

Uh oh!

dongjoon-hyun commented Jun 6, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gengliangwang commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jun 6, 2019

Uh oh!

SparkQA commented Jun 6, 2019

Uh oh!

Uh oh!

SparkQA commented Jun 7, 2019

Uh oh!

gengliangwang commented Jun 7, 2019

Uh oh!

SparkQA commented Jun 7, 2019

Uh oh!

gengliangwang commented Jun 8, 2019

Uh oh!

dongjoon-hyun commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Jun 8, 2019

Uh oh!

SparkQA commented Jun 8, 2019

Uh oh!

dongjoon-hyun commented Jun 8, 2019

Uh oh!

SparkQA commented Jun 8, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

jzhuge commented Jun 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gengliangwang commented Jun 6, 2019 •

edited

Loading

dongjoon-hyun Jun 6, 2019 •

edited

Loading

gengliangwang commented Jun 6, 2019 •

edited

Loading

dongjoon-hyun commented Jun 8, 2019 •

edited

Loading